Repository navigation
feat(core): WebKit plays bare audio from a decoded buffer on the audio clock - #6912
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed at 94e28188 (stacked on #6888 at 81653923). Requesting changes for one regression. The handover itself looks right.
Blocker: a speed change mid-play pitch-shifts a handed-over track. The body says non-unit rates stay on the element to keep pitch, but that is only checked when the track is scheduled (state.playbackRate === 1 && decodesFirst(rawEl)). Once the buffer has taken over, a Studio speed change goes through applyWebAudioRate(), which calls webAudio.setRate(). For an unbounded source, setRate() just sets sourceNode.playbackRate in place, and an AudioBufferSourceNode does not preserve pitch. The stopAll() and reschedule (which would put it back on the element, since the rate is no longer 1) runs only when hasBoundedActiveSources() is true. A bare <audio data-start> with no data-duration and no bounding host has clipDuration = Infinity, so it is unbounded. That is the case this PR targets.
Repro: in Safari, play a composition whose only audio is a bare <audio data-start> music bed, wait for the handover (~35 ms), then set speed to 1.5×. With this PR the bed plays chipmunked. On #6888 it stays pitch-corrected on the element.
Suggested fix: when a source was handed over by decodesFirst, treat a rate change away from 1 like the bounded case (stopAll() and reschedule), so it goes back to the element. Please add a test: hand over, change rate, and expect the element route to be rescheduled with no buffer schedulePlayback call.
Checked and fine:
releaseRouted→dispose(routed)reconnects the element's source todestination, butschedulePlaybackhas already setel.muted = true, so you don't hear both. When the buffer ends,handleSourceEndedrestorespriorMutedas the body describes.decodeAudioElementre-checks the generation andisPlayingbefore scheduling, andschedulePlaybackchecks the generation again afterwake(), so a seek or pause during the decode does not leave a stray buffer.- fails without the fix: with
init.tsreverted to the #6888 base, "hands the sound to a decoded buffer on the context clock once it is decoded" fails and the Chrome test passes. With the change, both pass.
Non-blocking:
!(el.duration > 300)lets an element whose duration is stillNaN(metadata not loaded) through to a full decode, with no size check. Consider requiringNumber.isFinite(el.duration).- The Safari audio and perf workflows were still running when I reviewed.
|
Fixed at ca10d37: a speed change now sends a handed-over track back to its element (stopAll and reschedule, so decodesFirst fails at the new rate and no buffer is scheduled). The test hands over, sets 1.5x and expects the element route again with no buffer schedulePlayback; it fails without the init.ts condition. Also took the NaN-duration point: decodesFirst now needs a known duration of at most 300 s. |
There was a problem hiding this comment.
Re-reviewed at ca10d370. Approving.
- Blocker resolved:
releaseRoutednow marks the element as handed over, andapplyWebAudioRate()stops and reschedules whenhasHandedOverSources()is true, not only when bounded sources are active. The reschedule runs at the new rate, sodecodesFirstis skipped (state.playbackRate !== 1) and the track goes back to its pitch-preserving element.stopAll()restoresel.mutedfrom the buffer source, and the element was still playing muted in sync, so it resumes where it should. - Duration gate:
el.duration <= 300now rejects aNaNduration, so no full decode starts before metadata has loaded. - Tests:
init.test.tsandwebAudioTransport.test.tspass 388/388 at this head. Withinit.tsreverted to the previous head, "returns a handed-over track to its element when the speed changes, to hold its pitch" fails. Thefails without the fix:test named in the body was verified at the previous head. - CI at this head: Comments, Player Safari audio, Player perf and preview-regression had passed; regression and Player Safari perf were still running.
Non-blocking:
- The new test stubs
hasHandedOverSourcesto return true, so it doesn't check thatreleaseRouted→_handedOver→ a live buffer source actually reports true. A smallWebAudioTransportunit test would pin that. - The body's
fails without the fix:line could also name the new speed-change test.
The base branch was changed.
Edit accuracy: accurate 271 (base branch 271), smooth 98 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (3)
|
ca10d37 to
f75dfea
Compare
There was a problem hiding this comment.
Re-reviewed at f75dfea0, the rebase onto main after #6888 merged. Approving: I confirmed this is a pure rebase.
- Same patch: the merge-base with
mainis #6888's merge commit2c1a10f3. The diff from that merge-base tof75dfea0is byte-identical (ignoringindexlines) to the diff I approved,81653923..ca10d370. - Same base code:
packages/core/src/runtime/is identical between #6888's old head81653923and its merge commit2c1a10f3, so the patch now sits on the same code it was reviewed against. - Tests:
init.test.tsandwebAudioTransport.test.tspass 388/388 at this head. - CI: the reruns for this head were still in progress when I reviewed (Comments and PR captures had passed).
My earlier review of ca10d370 covers the code; its non-blocking notes still apply.
…view to heygen-com#7250 Adopts heygen-com#7310 (MPEG-4 Part 2 / MJPEG get browser-playable copies), heygen-com#7316 (check_report counts the copies a check waited for) and heygen-com#7317 (render reads a composition src like the preview and refuses paths outside the project). Defers heygen-com#7309 (needs heygen-com#6912); heygen-com#7314 does not apply. PR axis advances to heygen-com#7250 (heygen-com#7243 closed unmerged, heygen-com#7249 open and pinned as monitored; monitored 96->97). Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
In WebKit, a bare
<audio data-start>starts on its element as today and, as soon as its decoded buffer is ready, moves to anAudioBufferSourceNodeon the AudioContext clock. The film clock then follows that context clock, so audio no longer has an element start delay, seeks or drift to tolerate. Follows #6888, which is merged.Why
#6888 stops the seek loop by tolerating up to 0.65 s of audio lag, the same trade Remotion makes. WebKit restarts an element's output 150 to 400 ms after play() or a seek, so any element route has that lag. A buffer source starts at the exact context time and needs no element restart.
How
init.ts:decodesFirst(el)selects WebKit audio at rate 1 that does not loop and is at most 300 s (decode memory cap). After the element route is scheduled, the decoded buffer is scheduled at the playhead.webAudioTransport.ts:releaseRouted(el)drops the element's routed source once the buffer plays; the element stays muted by the existing buffer path and is native again when the buffer ends.Numbers
3-take edit, old Mac WebKit (WKWebView), media on the same machine, 5 plays each, interleaved.
<Player>The seeks and frozen columns watch the audio element. In this PR the element is muted once the buffer plays, so they do not describe what is heard. What is heard, from the transport's own log over the same 5 plays:
Not measured: a loopback recording of the output, so the audible numbers above come from the transport log and the context's reported latency, not from captured sound. Safari proper (this is the same engine in a WKWebView) and iOS (gesture unlock follows the existing transport
wake()).Tests
init.test.ts> "bare audio in WebKit" > "hands the sound to a decoded buffer on the context clock once it is decoded"init.test.ts> "leaves the element on its route elsewhere" pins Chrome.Size
100 changed lines, a separate and independently revertable behaviour on top of #6888, whose tolerance this removes in practice.